fix(mcp): drop previous-transport keys on Claude server redeclaration by edenfunf · Pull Request #3041 · microsoft/apm

Eden (edenfunf) · 2026-09-20T10:26:00Z

fix(mcp): remove stale Claude transport fields without losing user configuration

TL;DR

Fix #2994's same-name Claude MCP redeclaration: HTTP-to-stdio removes stale
url/headers, and stdio-to-HTTP removes stale command/args/env/cwd.
The existing Claude merge path also repairs mixed entries when rewritten,
preserves partial updates and unmanaged configuration, and now has real CLI
lifecycle coverage in project and user scopes. Repeated writes also preserve
compatible field order, avoiding native JSON byte churn.

Important

Repair happens when APM writes the server. An unchanged self-defined
declaration with matching lock state can skip that write; reinstalling
unchanged is not an automatic mixed-entry migration.

Original implementation and mixed-entry fix: Eden (@edenfunf). The follow-up preserves
those commits and adds the partial-update and byte-order corrections with
lifecycle evidence.
Issue: #2994, already accepted; this does not replace required human review.

Problem (WHY)

The old shallow merge retained remote URL/header fields after a stdio
redeclaration, including the stale header reported in [BUG] Claude MCP entry keeps the old url/headers when a server's transport changes from http to stdio #2994.
A surviving URL also classified the merged entry as remote, preventing
Claude's type: "stdio" normalization. The reverse transition retained
stdio-only fields.
[!] Comparing old/new transport classification missed already-mixed entries.
Unconditionally pruning without preserving partial-update semantics then
lost HTTP connection settings or an omitted SSE type.
Approved CI exposed a further repeat-write regression: removing and
reinserting a compatible type moved it after url. Parsed values matched,
but the existing registry lifecycle's exact native bytes changed.

These are concrete state transitions, not a claim of observed credential
disclosure. The validation process follows Agent Skills'
"do the work, run a validator (a script, a reference checklist, or a self-check), fix any issues, and repeat until validation passes."
The executed regression mutations below establish which guards matter.

Approach (WHAT)

Share remote-family classification between merge cleanup and normalization.
For an actual declaration, remove the opposite family's fields even when
the old entry already contains both families.
For partial updates, retain the existing family and omitted compatible type.
Retain compatible type in its existing insertion position so unchanged
valid rewrites do not reorder native JSON.
Preserve the existing shallow merge, unrelated servers and both scope paths.
Leave shared install selection and lockfile drift detection unchanged.

Implementation (HOW)

File	Intent
`src/apm_cli/adapters/client/claude.py`	Filter old opposite-family fields before merging, preserving partial-update semantics and compatible type position in the existing shared helper.
`tests/unit/test_claude_mcp.py`	Adapter file-I/O regressions for transitions, mixed repair, passthrough state and six project/user partial-update cases, now including raw-byte repeats and key order.
`tests/integration/test_claude_mcp_transport_lifecycle.py`	Eight hermetic real-CLI cases spanning both scopes and both transport families, including denial, repair and stable repeats.
`docs/src/content/docs/consumer/install-mcp-servers.md`	Document preservation, stable valid rewrite order and repair-on-write, including the unchanged-install exception.
`packages/apm-guide/.apm/skills/apm-usage/commands.md`	Carry the same boundary into the companion install guidance.
`CHANGELOG.md`	Record the bounded fix under Unreleased and credit Eden (@edenfunf).

Diagrams

The dashed helper is the new cleanup stage; the existing no-write route remains unchanged.

flowchart LR
    subgraph Install["Install selection"]
        W{"Write required?"}
        K["Keep existing native config"]
    end
    subgraph Merge["Claude adapter"]
        R["_retained_previous_entry"]
        M["_merge_mcp_server_dicts"]
        N["_normalize_mcp_entry_for_claude_code"]
    end
    subgraph Output["Native config"]
        O["Write project or user JSON"]
    end
    W -->|No| K
    W -->|Yes| R
    R --> M
    M --> N
    N --> O
    classDef new stroke-dasharray: 5 5;
    class R new;

The block was parsed and rendered with Mermaid CLI before publication.

Trade-offs

Repair on write, not migration. Shared MCP reconciliation is outside the
accepted adapter scope. Tests deliberately preserve the unchanged-install
skip instead of silently expanding it.
Shallow preservation, not a new merge policy. Incoming fields replace
matching old fields; omitted compatible values and unmanaged keys survive.
This does not introduce recursive merging or normalize unrelated servers.
Real Python CLI, not a frozen binary or live MCP session. Subprocesses
use this exact worktree's editable installation with isolated HOME/config,
cache and credentials. Tests inspect actual JSON and lock state; they do not
launch MCP servers or validate a packaged executable.

Benefits

Both transport directions produce only their intended native field family.
Both scope paths retain OAuth, unrelated servers, top-level preferences and
nested project settings without writing the opposite scope.
Frozen denial and repeat installs preserve the recorded durable state.
Headers-only HTTP, URL-only SSE and args-only stdio updates preserve
compatible omitted settings and field order.

Validation

Evidence revision: 14578fc1ec57b1d5a2db75029c91ed61c63e2251.
Merged base: d68f052e60593a51fa1da2ce90a60f4da31f8398.

All six authorized exact-head workflows completed successfully:
CI,
Merge Gate,
Docs,
CodeQL,
NOTICE and
Spec.
Lifecycle Smoke, both test shards, Windows compatibility, lint, architecture
ratchets and PR Binary Smoke passed. Docs deployment was skipped as expected;
the aggregate CodeQL check was neutral. Required human review remains
outstanding; GitHub reports MERGEABLE/BLOCKED. No merge or auto-merge was
performed.

Executed local checks and regression mutations

The independent coverage reviewer executed the following selection through
the isolated pytest wrapper, with JUnit output retained in the driver's
evidence artifacts. Local registry fixtures require the two environment
variables shown here:

APM_E2E_TESTS=1 APM_BINARY_PATH="$PWD/.venv/bin/apm" \
uv run --frozen --extra dev python -m pytest -p no:cacheprovider -q \
  tests/unit/test_claude_mcp.py \
  tests/integration/test_claude_mcp_schema_fidelity.py \
  tests/integration/test_claude_mcp_transport_lifecycle.py \
  tests/integration/test_mcp_dev_dependency_lifecycle.py

65 passed, 4 subtests passed in 710.90s (0:11:50)

No failures, errors or skips. This includes all eight self-defined CLI
cases, three existing registry lifecycle cases and six strengthened
partial/repeat cases. The registry tests and their expected bytes were not
changed to accommodate the fix.

All seven current CI-mirror lint gates and architecture boundaries passed
locally on this head. The deterministic exact-base/head owner detector
reported no registered canonical-owner touch.

Deliberate mutation	Observed result	Provenance
Remove opposite-family pruning	13 failures, including all eight CLI lifecycle cases; one preservation control passed.	`8ad164d9`
Restore obsolete same-classification fast path	Both project/user remote mixed-entry CLI cases failed.	`8ad164d9`
Remove partial-update family/type preservation	Four HTTP/SSE cases failed; two stdio controls still passed.	`8ad164d9`
Remove and reinsert compatible type	Eight raw-byte assertion failures: all six adapter repeat cases and both CI-failing registry cases.	Recovery working tree before `11aea75c`

All mutations were restored. The recovery commit 11aea75c passed
128 tests plus 4 subtests, including tests/quality, before the main
synchronization. The current head has its own fresh 65-test plus 4-subtest
run and successful CI; historical runs and mutations are not relabeled
as current-head executions.

Scenario Evidence

The self-defined lifecycle cases run apm install --target claude --no-policy
through the real Python CLI, parametrized over project and --global scopes.
The existing registry cases use isolated local registry/Git fixtures and the
installed CLI entrypoint.

#	Scenario (user promise)	Principle(s)	Test(s) proving it	Type
1	Change HTTP to stdio and remove the old URL/header fields.	Portability by manifest, Multi-harness support	`tests/integration/test_claude_mcp_transport_lifecycle.py::test_transport_redeclaration_preserves_unowned_state` with initial `http` (regression-trap for #2994)	e2e
2	Change stdio to HTTP without stale command/args/env/cwd.	Portability by manifest	Same test with initial `stdio`.	e2e
3	Repair an already-mixed entry when changing its declaration.	Portability by manifest, DevX (pragmatic as npm)	`tests/integration/test_claude_mcp_transport_lifecycle.py::test_legacy_mixed_state_is_repaired_only_on_redeclaration` (regression-trap for #2994)	e2e
4	Leave unchanged legacy mixed state alone rather than promise migration.	DevX (pragmatic as npm)	`tests/integration/test_claude_mcp_transport_lifecycle.py::test_legacy_mixed_state_is_repaired_only_on_redeclaration`	e2e
5	Reject a stale frozen declaration without changing native config or durable state.	Governed by policy	`tests/integration/test_claude_mcp_transport_lifecycle.py::test_transport_redeclaration_preserves_unowned_state`	e2e
6	Preserve OAuth, unrelated state and the opposite scope during an update.	Secure by default, Multi-harness support	Both tests in `tests/integration/test_claude_mcp_transport_lifecycle.py`.	e2e
7	Repeat a converged install without config or lock-state churn.	DevX (pragmatic as npm)	Both tests in `tests/integration/test_claude_mcp_transport_lifecycle.py`.	e2e
8	Update only HTTP headers, an SSE URL or stdio args without losing other connection settings.	DevX (pragmatic as npm)	`tests/unit/test_claude_mcp.py::test_partial_updates_preserve_transport`	integration
9	Repeat actual adapter writes without changing native bytes or compatible field order.	DevX (pragmatic as npm)	`tests/unit/test_claude_mcp.py::test_partial_updates_preserve_transport`, all six scope/transport cases (CI regression-trap).	integration
10	Install, repeat, update and reinstall registry-backed Claude entries with unchanged expected native bytes.	DevX (pragmatic as npm)	`tests/integration/test_mcp_dev_dependency_lifecycle.py::test_dependency_dev_mcp_isolated_across_installed_lifecycle[claude]` (existing CI regression-trap).	e2e
11	Deny frozen stale-registry repair without writes, then repair through install/update while preserving user entries.	Governed by policy, DevX (pragmatic as npm)	`tests/integration/test_mcp_dev_dependency_lifecycle.py::test_stale_dependency_dev_mcp_repair_and_frozen_no_write` (existing CI regression-trap).	e2e

How to test

Check out the evidence revision and run the pytest command above;
expect all selected cases to pass without skips.
Inspect the native-config assertions in the new lifecycle module:
both scopes and directions must remove only the opposite transport fields.
Inspect the frozen and repeat phases: expect denial before mutation,
and byte-equal native config plus equal lifecycle snapshots on repeats.
Compare the unchanged mixed-state phase with its declaration-change
phase: only the latter repairs the entry.
Inspect raw-byte assertions in the six partial/repeat cases and both
existing registry regression cases; dictionary equality alone is insufficient.
Verify the linked successful workflows still match the current head;
retain human CODEOWNER review before any merge.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Claude Code server entries were shallow-merged as {**old, **new}, so a server redeclared under another transport kept the keys of the transport it left behind. The surviving url also re-classified the entry as remote, so the stdio normalisation never ran and a stale Authorization header stayed in the Claude Code config. Drop the keys describing the replaced transport before the merge, and share one remote/stdio classifier between the merge and the normalizer so both read an entry the same way. Keys APM does not manage, such as hand-authored OAuth blocks, describe no transport and still survive. Fixes microsoft#2994

Copilot

Copilot review overview

🟡 Changes recommended

Existing mixed configurations can retain stale stdio keys, and the new runtime-config behavior is not documented.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

Clean stale transport keys in unchanged mixed entries · New
Document transport-key cleanup for same-name redeclarations · New

What changed in this PR

Fixes Claude MCP transport redeclarations so stale transport-specific fields are removed.

Changes:

Adds transport-aware merge cleanup and shared classification.
Adds regression tests for transport switches and key preservation.
Records the fix in the changelog.

File	Description
`src/apm_cli/adapters/client/claude.py`	Implements transport-aware merging and normalization.
`tests/unit/test_claude_mcp.py`	Tests transport changes and shallow-merge behavior.
`CHANGELOG.md`	Documents the fix.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Selecting the stale keys by comparing the stored entry's transport to the update's left entries written by an earlier release unrepaired: such an entry carries a url, so it classifies as remote, and a remote update was read as no transport change at all. The stdio keys, an env block among them, survived every reinstall. Select the stale keys from the update alone. An entry is then rewritten to carry only the transport its declaration names, whatever shape it had before, and the comparison branch disappears. Also document the rule where the guide describes what install writes to disk. Claude Code is the only target merging entries key by key, so it is the only one where a stored key can outlive its declaration.

Sergio Sisternes (sergio-sisternes-epam) · 2026-09-23T17:49:57Z

Thank you for this pull request. It implements the accepted scope on #2994. CODEOWNERS review is already requested of danielmeppiel and sergio-sisternes-epam; that request is unchanged. This comment is advisory only and is not merge approval. Please wait for a human maintainer to review.

Generated by autopilot-pr-triage-worker. This comment is AI-generated and may contain errors.

Synchronize the accepted transport fix with current main without rewriting contributor commits. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Daniel Meppiel (danielmeppiel) · 2026-09-30T07:26:23Z

APM Review Panel: `needs_rework`

Claude transport cleanup has the right scope, but partial remote updates regress and real-CLI preservation evidence is still missing.

panel-mode=lean; personas=python-architect,test-coverage-expert,apm-ceo

cc Eden (@edenfunf) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

This initial lean advisory assesses local, main-synchronized candidate 0bcf414, not the live PR head 91b2a01; the candidate has not been pushed. Both authors' fixes are present, including cleanup of already-mixed entries on explicit writes and the guide addition. The two unresolved historical Copilot threads are not two fresh implementation defects. The specialists agree that the existing shared Claude adapter helper is the right owner and that unrelated configuration must remain untouched; neither recommends expanding shared no-op reconciliation.

The architect identifies a concrete, narrower preservation regression: a headers-only HTTP update loses its URL and becomes stdio, while a URL-only SSE update loses its explicit SSE type. The supplied isolated base-versus-candidate method comparison supports that diagnosis, but it is manual evidence, not an automated regression trap or a failing candidate test. Ordinary formatted CLI declarations supply an explicit type, so this finding does not establish that normal CLI redeclaration fails. Nevertheless, silently changing partial-update semantics is outside the intended cleanup promise. Preserve omitted same-family settings while retaining unconditional opposite-family pruning for actual transport declarations and mixed-entry rewrites.

Coverage confirms that adapter file-I/O traps already exist; they must not be described as absent or as passing. What is missing is real-CLI lifecycle evidence across both scopes for transitions, frozen denial without writes, preservation of unowned state, and stable repeats. The secure-by-default preservation gap warrants priority above documentation polish, without implying a demonstrated credential disclosure. Pair that evidence with an explicit legacy mixed-state scenario: unchanged manifest/lock state may skip writing, whereas a real declaration change repairs the entry. Align the guide and helper docstring with the changelog's existing 'the next install that writes it' qualification. No current automated tests or CI have run for this candidate; historical author claims and explained unknown outcomes do not establish candidate success.

Aligned with: portable by manifest: A changed Claude declaration should produce the intended native transport shape; the missing CLI lifecycle evidence leaves that end-to-end promise unverified. secure by default: Removing stale transport fields while preserving OAuth and unrelated state is the right boundary, but both-scope preservation needs an automated lifecycle guardrail. multi harness multi host: Keep Claude-specific normalization in the shared Claude adapter helper used by project and user writes, without introducing cross-target reconciliation. pragmatic as npm: Preserve partial-update compatibility, prove stable repeat installs, and document repair-on-write rather than promising automatic migration on an unchanged install.

Panel summary

Persona	B	R	N	Takeaway
python-architect	0	2	0	Adapter-local cleanup fits the ownership boundary; preserve partial remote updates and explicitly document repair-on-write. No automated tests were run.
test-coverage-expert	0	2	0	Adapter regression traps exist, but Claude transport transitions and legacy repair/no-op boundaries lack real-CLI lifecycle evidence on this candidate.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 3 follow-ups

[test-coverage-expert] Add and execute one bounded Claude real-CLI lifecycle module covering transport changes and the repair-on-write boundary in project and user scopes. -- The missing secure-by-default preservation evidence is the highest-priority test gap. Use the real candidate engine and hermetic local declarations without mocking install, formatting, or adapter writes. Cover both transport directions, frozen stale-lock denial with unchanged snapshots, OAuth/unrelated/top-level/nested-project preservation, opposite-scope isolation, and byte-stable repeats. In the same module, prove that unchanged legacy mixed state is left alone, then repaired by an actual same-family declaration change. Verify the new traps detect removed pruning and restoration of the obsolete same-classification fast path. Existing adapter tests are useful lower-tier coverage, not a substitute for these lifecycle assertions.
[python-architect] Preserve partial remote-update semantics and add automated headers-only HTTP and URL-only SSE regression traps. -- The isolated comparison shows a compatibility regression even though ordinary formatted declarations supply a type. Distinguish partial updates from explicit transport declarations, retain omitted same-family connection settings and type, and keep pruning on actual declarations so already-mixed entries still repair. This is a bounded correction inside the existing Claude helper, not a reason to change MCPIntegrator.
[python-architect] Make the guide and helper docstring explicitly promise repair only when the server entry is written. -- The changelog already qualifies the behavior correctly. Repeating an unchanged self-defined installation with matching lock state can skip the adapter write and leave legacy mixed JSON untouched. State that limitation consistently; resolving documentation ambiguity must not expand the accepted no-op reconciliation scope.

Recommendation

I recommend a bounded in-PR revision: correct the partial-update regression, add and execute the both-scope lifecycle evidence, and align repair-on-write wording before returning for human review. This recommendation follows the concrete preservation defect and missing critical guardrail, not the historical thread status or a demand for automatic no-op repair. Reassess the resulting exact candidate with observed test and CI results; this local advisory grants no merge approval and leaves CODEOWNERS danielmeppiel and sergio-sisternes-epam in place.

Full per-persona findings

python-architect

[recommended] Partial remote updates lose their existing transport and connection details at src/apm_cli/adapters/client/claude.py:140
The merge previously preserved omitted keys, including type. _retained_previous_entry now treats any update without a URL or remote type as stdio, and both pruning sets unconditionally remove type. An isolated execution of the exact merge/normalization methods from base and candidate confirmed two regressions: updating an existing HTTP entry with only headers previously retained its URL and HTTP type, but now produces only headers plus type=stdio; updating an existing SSE entry with only url previously retained type=sse, but now drops type entirely. This conflicts with the shallow-merge preservation contract. The normal configure_mcp_server path currently receives an explicit type from CopilotClientAdapter._format_server_config, so this is a partial-update API regression, not evidence that ordinary formatted CLI redeclarations fail. The shared Claude helper remains the appropriate owner; no additional abstraction or integrator change is needed.
Suggested: Distinguish an explicit transport declaration from a partial update. When the update supplies no transport discriminator, preserve the previous family; preserve an omitted same-family type as well. Keep unconditional opposite-family pruning for actual declarations so already-mixed entries still repair. Add headers-only HTTP and URL-only SSE preservation regressions.
Proof (manual, manual-only): :: -- Updating only a remote server's headers or URL no longer preserves its omitted connection settings.
[recommended] Make the unchanged-install exception explicit in the guide at docs/src/content/docs/consumer/install-mcp-servers.md:137
The guide describes transport cleanup but does not explain when an existing mixed entry is actually rewritten. _check_self_defined_servers_needing_installation checks name presence, _detect_mcp_config_drift compares the declaration against stored lock state, and _install_self_defined_deps skips names absent from its install list. Consequently, an unchanged declaration present in every target with matching lock state can leave a legacy mixed entry untouched. CHANGELOG.md:23 correctly says 'the next install that writes it'; the helper docstring at claude.py:133-136 instead says 'the next install'. The approved no-op reconciliation boundary is coherent and should remain unchanged, but readers need the same repair-on-write qualification in the guide and helper documentation.
Suggested: State explicitly that cleanup occurs only when APM writes that server entry, and that repeating an unchanged self-defined installation with matching lock state may skip the write and therefore not repair an older mixed entry. Align the helper docstring with the changelog's existing qualification.

test-coverage-expert

[recommended] Exercise transport redeclaration through the real CLI in both project and user scopes. at tests/unit/test_claude_mcp.py:293
The six added TestClaudeTransportChange tests use real adapter file writes, but all construct user_scope=False and bypass declaration loading, lock drift detection, and install routing. The golden-schema tests cover fresh project/user writes and user-config preservation, not same-name transport changes. Narrow glob/rg probes of tests/unit/mcp.py, tests/integration/mcp.py and tests/integration/lifecycle.py found the transition traps only in test_claude_mcp.py. Read the matching lifecycle tests: test_required_mixed_primitives_survive_reinstall_without_state_loss does not change transport, and test_frozen_stale_mcp_lock_is_read_only_and_normal_install_repairs changes Cursor args rather than Claude transport. Thus lower-tier regression tests are present, but the user-facing transition and its preservation boundary lack an ApmLifecycle contract. This is a coverage gap, not an observed correctness or credential-disclosure failure; a bounded hermetic fixture path exists.
Suggested: Add test_transport_redeclaration_preserves_unowned_state to the proposed tests/integration/test_claude_mcp_transport_lifecycle.py, parametrized over project/user scope and HTTP->stdio/stdio->HTTP. Use ApmLifecycleRunner with a candidate-built engine, IsolatedApmEnvironment, and local registry:false declarations; do not mock install, formatting, or adapter writes. Sequence: apm install --target claude --no-policy [--global]; change the same-name declaration; snapshot; install with --frozen and the same scope/target options; assert nonzero exit, a diagnostic identifying the server/config mismatch, and unchanged native-config/lock snapshots; normal install; repeat normal install. Assert exact native entry shape: stdio type with no url/headers, or HTTP type with no command/args/env/cwd. Seed and compare OAuth/passthrough values, unrelated servers including an untouched mixed entry, top-level keys, nested projects, and the opposite-scope sentinel. Require byte-stable config and lock state on the final repeat. Include a same-family stdio update preserving an omitted cwd. Fold the frozen negative phase into these cases rather than adding a separate expensive suite. A cohesive Claude-specific module is preferable to expanding the unrelated mixed-primitives lifecycle test: this needs two scope roots and native-config sentinels. Apply the integration/e2e/lifecycle_smoke/requires_apm_binary/requires_e2e_mode marker contract. Keep the target bounded to Claude; no cross-target reconciliation is requested.
Proof (missing, e2e): tests/integration/test_claude_mcp_transport_lifecycle.py::test_transport_redeclaration_preserves_unowned_state -- Changing a Claude server's transport updates only that server's transport fields, in the selected scope, without damaging unrelated configuration.
[recommended] Pin the legacy mixed-entry repair-on-write boundary without requiring automatic no-op migration. at tests/unit/test_claude_mcp.py:374
The direct adapter tests test_mixed_entry_from_earlier_release_is_repaired and test_mixed_entry_repaired_towards_stdio correctly exercise explicit rewrites; they cannot prove that an unchanged CLI install reaches the adapter. Read _install_self_defined_deps, _check_self_defined_servers_needing_installation, and _detect_mcp_config_drift: name presence plus matching manifest/lock state can skip writing even when native JSON is mixed. Narrow searches for mixed_entry, legacy mixed, transport transitions and no-op coverage found no real-CLI lifecycle for this distinction. Matching integrator tests either mock these seams or assert their return values in isolation. The accepted limitation is therefore supported by source inspection, not by an executed consumer scenario. The requested test should document the unchanged mixed-state skip, not turn it into a new reconciliation requirement. Severity is recommended because no in-scope implementation failure was reproduced.
Suggested: Add test_legacy_mixed_state_is_repaired_only_on_redeclaration to the same proposed lifecycle module, sharing its scope fixtures rather than creating another module. In project and user scopes, install a self-defined declaration to establish matching lock state; plant a legacy mixed native entry while preserving the manifest and lock; snapshot; run the identical install and assert byte-for-byte unchanged native config and durable state. Then change a real same-family declaration field (URL for remote, args for stdio), run install, and assert removal of the opposite family's keys with passthrough/unrelated state preserved; repeat and assert byte stability. Cover rewrites toward both families. The remote case must start with both url and stdio keys, so the old same-classification fast path would fail the test. Later mutation verification should show the transition test fails when pruning is removed and this remote mixed-rewrite case fails when the obsolete fast path is restored. No source mutation was performed in this review.
Proof (missing, e2e): tests/integration/test_claude_mcp_transport_lifecycle.py::test_legacy_mixed_state_is_repaired_only_on_redeclaration -- An unchanged install does not promise to migrate legacy mixed Claude state; an actual redeclaration repairs that entry and subsequent installs remain stable.

_{This panel is advisory. It does not block merge. Re-apply the

panel-review label after addressing feedback to re-run.}

Generated by autopilot-pr-review-worker. This comment is AI-generated and may contain errors.

Address the initial panel's partial-update regression and add real isolated CLI coverage for transport changes, mixed-state repair, no-op installs, frozen denial and user configuration preservation. Keep shared no-op reconciliation unchanged and document repair-on-write. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Daniel Meppiel (danielmeppiel) · 2026-09-30T07:57:27Z

APM Review Panel: `ship_now`

Claude's CI-discovered byte-order regression is corrected and exact-head CI now passes; transport cleanup stays bounded to actual writes, with human review still outstanding.

panel-mode=delta; personas=python-architect,test-coverage-expert,apm-ceo

cc Eden (@edenfunf) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The two CI byte-order failures in tests/integration/test_mcp_dev_dependency_lifecycle.py invalidated the prior 8ad164d ship_now assessment. This is a fresh assessment of pushed head 14578fc against current base d68f052. Recovery 11aea75 retains a compatible type at its original insertion position; incoming values still win, opposite-family fields are still pruned, and partial updates retain omitted compatible settings. Coverage independently executed 65 tests plus 4 subtests on this exact head, without skips: all eight self-defined CLI cases, all three registry lifecycle cases, and the six strengthened byte-repeat cases passed. In tests/unit/test_claude_mcp.py::test_partial_updates_preserve_transport, assert config_path.read_bytes() == initial_bytes now protects repeated writes. Both previously failing registry cases also pass their unchanged expected-native-byte assertions. The remove/reinsert mutation produced eight actual byte-assertion failures before restoration and commit as 11aea75, demonstrating that these traps detect the observed defect rather than merely parsed-JSON differences.

The specialists converge on no substantive correction remaining within the accepted scope. The existing Claude helper remains the owner; synchronized-owner.json records no registered canonical-owner touch across the six-file diff, and the supplied exact-head lint evidence reports all seven checks and architecture boundaries passing. The architect's no-change design nit is informational, not an action item; its unsolicited diagrams do not belong in this delta. The no-op boundary remains deliberate: an unchanged self-defined declaration with matching lock state may skip the adapter and leave legacy mixed JSON untouched. In tests/integration/test_claude_mcp_transport_lifecycle.py::test_legacy_mixed_state_is_repaired_only_on_redeclaration, assert scenario.snapshot(scenario.user_scope) == selected protects that boundary; actual redeclaration still repairs the entry. The registry frozen-denial test retains assert _snapshot(scenario) == stale. Local lifecycle evidence uses isolated editable Python CLI processes and local fixtures, not live MCP services or general policy certification. The 128-test plus 4-subtest local quality-inclusive run belongs to 11aea75, not this synchronized head; older mutations retain their original provenance and were not rerun here.

The newer saved API snapshots supersede the specialists' earlier pending-CI caveats: all six authorized exact-head workflows completed successfully, including CI 36691826793, readiness workflow 36691826759, Docs 36691826932, CodeQL 36691826799, NOTICE 36691826780 and Spec 36691826843. The check rollup independently records successful Lifecycle Smoke, both Linux test shards, Windows compatibility, lint, architecture ratchets, PR Binary Smoke and gate; docs deployment is skipped and the aggregate CodeQL check is neutral, not failed. This conclusion comes from recorded workflow and check outcomes, not a watcher exit or local-test inference. PR Binary Smoke success does not turn the local lifecycle matrix into packaged-binary or live-service coverage. Human review is still outstanding: no approving review appears, and CODEOWNERS danielmeppiel and sergio-sisternes-epam remain requested. Authorization covered workflow execution only, not PR approval, merge or auto-merge. The inspected published PR body still describes 8ad164d and pending workflows; its orchestrator-owned refresh is pending, not already verified. Software readiness is supported anew, while publication accuracy and the existing human-review process remain separate responsibilities.

Dissent. No substantive disagreement remains. The architect's single nit explicitly requests no design change, so it is informational and creates no follow-up; delta rendering must omit its diagrams. Its earlier pending-validation caveats are superseded only where the newer exact-head coverage report and saved workflow snapshots supply results.

Post-synthesis publication update (orchestrator). The PR body has now been refreshed to 14578fc1 and verified by reading it back. The review-time pending-publication caveat above is therefore resolved. The architect's earlier pending-CI caveat below is superseded by the successful CI run and readiness workflow, as assessed in the synthesis. Required human review remains outstanding; no approval, merge or auto-merge was performed.

Aligned with: portable by manifest: Executed both-scope CLI transitions remove opposite-family fields on actual writes without promising migration on unchanged installs. secure by default: The retained lifecycle checks protect OAuth, unrelated configuration and the opposite scope; this is bounded preservation evidence, not a general security certification. governed by policy: Frozen-denial snapshots remain unchanged, and successful authorized workflow execution does not substitute for human review or authorize merge. oss community driven: The bounded recovery preserves Eden (@edenfunf)'s contribution and existing CODEOWNER requests while explicitly correcting the earlier readiness assessment. pragmatic as npm: Compatible partial updates and byte-stable repeated writes are regression-protected without expanding shared reconciliation.

Panel summary

Persona	B	R	N	Takeaway
python-architect	0	0	1	`14578fc` fixes compatible-type byte churn without changing merge values. No substantive architecture finding; synchronized full-suite evidence and exact-head GitHub CI remain outstanding.
test-coverage-expert	0	0	0	At `14578fc`, 65 tests and 4 subtests passed. Byte-order regression traps pass without weakening expected bytes, pruning, or partial updates. No coverage findings.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Recommendation

I recommend ship_now as a software advisory for 14578fc: the CI-discovered regression is corrected, exact-head tests and workflows pass, and no additional substantive in-scope follow-up is identified. The orchestrator should publish the corrected head-specific evidence, then leave the decision with the existing human reviewers. This is neither PR approval nor permission to merge or enable auto-merge; any subsequent head change or new failure requires reassessment.

Full per-persona findings

python-architect

[nit] The existing adapter structure remains appropriate; no design change requested at src/apm_cli/adapters/client/claude.py:142
Design patterns
Used in this PR: Adapter + base/subclass -- ClaudeClientAdapter reuses CopilotClientAdapter formatting while retaining Claude-specific merge and normalization in the existing helper chain.
Pragmatic suggestion: none -- the current shape is the simplest correct design at this scope.

test-coverage-expert

No findings.

_{This panel is advisory. It does not block merge. Re-apply the

panel-review label after addressing feedback to re-run.}

Generated by autopilot-pr-review-worker. This comment is AI-generated and may contain errors.

Keep compatible transport types in their original insertion position while pruning opposite-family fields. Strengthen raw-byte repeat and partial-update order assertions; preserve existing registry lifecycle snapshots. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI lite review requested due to automatic review settings September 20, 2026 10:26

Eden (edenfunf) requested review from Daniel Meppiel (danielmeppiel) and Sergio Sisternes (sergio-sisternes-epam) as code owners September 20, 2026 10:26

Copilot started reviewing on behalf of Eden (edenfunf) September 20, 2026 10:26 View session

Copilot AI reviewed Sep 20, 2026

View reviewed changes

Comment thread src/apm_cli/adapters/client/claude.py Outdated

Comment thread src/apm_cli/adapters/client/claude.py Outdated

Merge main into Claude MCP transport fix

0bcf414

Synchronize the accepted transport fix with current main without rewriting contributor commits. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Daniel Meppiel (danielmeppiel) mentioned this pull request Sep 30, 2026

fix(mcp): write env-sourced MCP headers into Codex's runtime fields #3042

Merged

Daniel Meppiel (danielmeppiel) and others added 2 commits September 30, 2026 10:16

Merge main into Claude transport recovery

14578fc

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Daniel Meppiel (danielmeppiel) linked an issue Sep 30, 2026 that may be closed by this pull request

[BUG] Claude MCP entry keeps the old url/headers when a server's transport changes from http to stdio #2994

Closed

Daniel Meppiel (danielmeppiel) merged commit 598014b into microsoft:main Sep 30, 2026
21 checks passed

Navigation Menu

fix(mcp): drop previous-transport keys on Claude server redeclaration - #3041

fix(mcp): drop previous-transport keys on Claude server redeclaration#3041
Daniel Meppiel (danielmeppiel) merged 6 commits into
microsoft:mainfrom
edenfunf:fix/2994-claude-mcp-transport-change

Eden (edenfunf) commented Sep 20, 2026 •

edited by danielmeppiel

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Sergio Sisternes (sergio-sisternes-epam) commented Sep 23, 2026

Uh oh!

Daniel Meppiel (danielmeppiel) commented Sep 30, 2026

python-architect

test-coverage-expert

Uh oh!

Daniel Meppiel (danielmeppiel) commented Sep 30, 2026 •

edited

Loading

python-architect

test-coverage-expert

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Eden (edenfunf) commented Sep 20, 2026 • edited by danielmeppiel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR

Problem (WHY)

Approach (WHAT)

Implementation (HOW)

Diagrams

Trade-offs

Benefits

Validation

Scenario Evidence

How to test

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Sergio Sisternes (sergio-sisternes-epam) commented Sep 23, 2026

Uh oh!

Daniel Meppiel (danielmeppiel) commented Sep 30, 2026

APM Review Panel: needs_rework

Panel summary

Top 3 follow-ups

Recommendation

python-architect

test-coverage-expert

Uh oh!

Daniel Meppiel (danielmeppiel) commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

APM Review Panel: ship_now

Panel summary

Recommendation

python-architect

test-coverage-expert

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Eden (edenfunf) commented Sep 20, 2026 •

edited by danielmeppiel

Loading

APM Review Panel: `needs_rework`

Daniel Meppiel (danielmeppiel) commented Sep 30, 2026 •

edited

Loading

APM Review Panel: `ship_now`